전남대Android_오진우_1주차 과제 #35
Conversation
|
안녕하세요 오진우님! 앞으로 6주동안 함께하게 될 멘토 강대규 입니다 :) 전반적으로 코드는 한번 살펴봤는데요, 오늘부터 목요일까지 일정이 있어 코드 리뷰는 금요일에 진행할 수 있도록 해볼게요! |
There was a problem hiding this comment.
리뷰가 늦어 죄송합니다 :) 수고 많으셨어요!
구현할 기능 목록을 작성할 때 어떤 기준으로 정해야 할 지 잘 모르겠습니다. 너무 작게 나누면 commit을 너무 자주하게 되 오히려 복잡해지는 것 같습니다.
커밋 단위를 잘 나눠주고 계세요! 아직 감이 잘 안오실 수 있습니다. 아직은 조급해하지 않으셔도 됩니다. 경험이 부족하여 개발 시에 어떤 단계로 진행하게 될 지 예측이 어려운 부분이 있어서 커밋단위를 나누기도 어렵다고 생각해요. 이부분은 경험이 채워줄 것이라고 생각합니다.
또한 터치 이벤트(OnClickListener)를 사용할 때 Button과 TextView를 언제 사용해야 할 지 잘 모르겠습니다. 둘의 차이가 궁금합니다.
Button 과 TextView 의 기능적인 차이는 존재하지 않습니다. Button 의 내부코드를 들여다보면 TextView 를 extends 하고있고, 특별한 처리는 하지 않고 있거든요.
큰 차이점이라고 하다면 Role 에 대한 부분과 테마관련 처리정도로 볼 수 있겠네요.
Intent를 활용하여 액티비티간 이동할 때 어려움을 느꼈던 것 같습니다.
잘 활용하고 계세요. 추가적으로 코멘트를 드린다면 Parcelable 과 Serializable 을 공부해보시면 좋을 것 같습니다 :)
RecyclerView가 효과적으로 구현되어 있는지
RecyclerView 는 동적으로 리스트가 변화하는 과정을 경험해보셔야 조금 더 와닿으실 것 같아요. 현재는 list 가 아래로만 쌓이고 있지만, 중간에 들어가는 경우나 특정 아이템이 변경되는 경우 등을 경험할 때는 현재 구조로는 힘듭니다. 이럴 때는 ListAdapter 를 사용해보시길 권장드려요.
| lateinit var name: TextView | ||
| lateinit var phoneNumber: TextView | ||
| lateinit var mail: TextView | ||
| lateinit var birthday: TextView | ||
| lateinit var gender: TextView | ||
| lateinit var memo: TextView | ||
| lateinit var nameInputField: TextView | ||
| lateinit var phoneNumberInputField: TextView | ||
| lateinit var mailInputField: TextView | ||
| lateinit var birthdayInputField: TextView | ||
| lateinit var genderInputField: TextView | ||
| lateinit var memoInputField: TextView |
There was a problem hiding this comment.
ViewBinding 을 통해서 findViewById 를 사용하지 않고 처리해볼 수 있어요 :)
| fun ExistMail(): Boolean { | ||
| return mailInputField.text.toString() != "" | ||
| } | ||
|
|
||
| fun ExistBirthday(): Boolean { | ||
| return birthdayInputField.text.toString() != "" | ||
| } | ||
|
|
||
| fun ExistGender(): Boolean { | ||
| return genderInputField.text.toString() != "" | ||
| } | ||
|
|
||
| fun ExistMemo(): Boolean { | ||
| return memoInputField.text.toString() != "" | ||
| } |
There was a problem hiding this comment.
함수명은 kotlin naming convention 에 따라 소문자로 시작하는 편이 좋습니다.
|
|
||
|
|
||
| fun ExistMail(): Boolean { | ||
| return mailInputField.text.toString() != "" |
There was a problem hiding this comment.
String 의 isEmpty 함수를 활용한다면 조금 더 명확하게 코드를 표현할 수 있을 것 같아요 :)
| mail.visibility = View.VISIBLE | ||
| mailInputField.visibility = View.VISIBLE |
There was a problem hiding this comment.
View.isVisible 을 통해 visibility 컨트롤이 더 쉽습니다.
| mail.visibility = View.VISIBLE | |
| mailInputField.visibility = View.VISIBLE | |
| mail.isVisible = true | |
| mailInputField.isVisible = true |
There was a problem hiding this comment.
또한 isVisible 을 활용한다면 아래처럼 if 분기를 태우지 않더라도 처리할 수 있습니다.
val isExistMailField = existMail()
mail.isVisible = isExistMailField
mailInputField = = isExistMailField
|
|
||
| fun initVar(){ | ||
| contactList = mutableListOf<Contact>() | ||
| adapter = RecyclerViewAdapter(contactList, LayoutInflater.from(this), this) |
There was a problem hiding this comment.
adapter 를 초기화하는 것 또한 recycler view 를 초기화하는 것과 같은 맥락이므로, initRecyclerView 로 이동하는 것은 어떨까요?
| nameInputField.setText(intent.getStringExtra("name")) | ||
| phoneNumberInputField.setText(intent.getStringExtra("phoneNumber")) | ||
| mailInputField.setText(intent.getStringExtra("mail")) | ||
| birthdayInputField.setText(intent.getStringExtra("birthday")) | ||
| genderInputField.setText(intent.getStringExtra("gender")) | ||
| memoInputField.setText(intent.getStringExtra("memo")) |
There was a problem hiding this comment.
intent 에서 공통으로 사용하는 key 들은 따로 관리하는 편이 좋습니다. 수동으로 복붙을 진행하다보면, 키 복사를 제대로 하지 못하는 등의 휴먼에러도 발생할 수 있으니까요.
There was a problem hiding this comment.
companion object 를 사용하여 const 로 관리해주시면 좋을 것 같네요 :)
class DetailActivity: AppCompatActivity() {
...
companion object {
const val KEY_NAME = "name"
}
}
| class RecyclerViewAdapter( | ||
| var contactList: MutableList<Contact>, | ||
| var inflater: LayoutInflater, | ||
| var context: Context |
There was a problem hiding this comment.
context 를 변수로 넘기는 것은 금기시해야할 것 중 하나입니다. inflater 또한 파라미터로 받을 필요는 없을 것 같구요.
아이템을 클릭했을 때 이동해야한다면, 아이템이 클릭되었음을 알려주는 listener 를 정의하고 그것을 파라미터로 받는 것이 좋습니다.
interface OnContactClickListener {
fun onContactClicked(position: Int)
}
| var context: Context | ||
| ) : RecyclerView.Adapter<RecyclerViewAdapter.ViewHolder>() { | ||
|
|
||
| inner class ViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) { |
There was a problem hiding this comment.
inner class 로 구현하지 않았으면 해요. 필요하다면 userTitle 과 userName 을 설정할 수 있는 함수를 만드는 편이 좋을 것 같습니다.
| const val EXIST_NAME_AND_PHONE_NUMBER = 1 | ||
| const val NOT_EXIST_NAME = 2 | ||
| const val NOT_EXIST_PHONE_NUMBER = 3 | ||
|
|
step1
코드 작성하면서 어려웠던 점
코드 리뷰 시, 멘토님이 중점적으로 리뷰해줬으면 하는 부분
step2
코드 작성하면서 어려웠던 점
코드 리뷰 시, 멘토님이 중점적으로 리뷰해줬으면 하는 부분